Add fanout params for mongodb planning stage - #384
Conversation
📝 WalkthroughWalkthroughThe PR introduces configurable concurrency limits for MongoDB connector planning and DocumentDB sampling operations. New ChangesMongoDB Connector Fanout Concurrency Control
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/app/options/connectorflags.go (1)
734-741: 💤 Low valueOptional: avoid the hardcoded
100default.The hidden flag duplicates the magic number
100, which can silently diverge frommongo.defaultDocumentDBSamplingFanoutLimit. Since the connector already applies that default inNewConnviasetDefault, you could drop the flag'sValue(or export/reuse the constant) so there is a single source of truth. Also note this flag is wired manually here whilenamespace-fanoutflows through the proto/generated path — intentional given it's hidden, but worth keeping in mind for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/options/connectorflags.go` around lines 734 - 741, The hidden flag in mongoFlagsCommandFlags currently hardcodes Value: 100 for "documentdb-sampling-fanout"; instead either remove the Value so the connector's NewConn/setDefault logic (mongo.defaultDocumentDBSamplingFanoutLimit) remains the single source of truth, or reuse/export that constant for the flag's default; update mongoFlagsCommandFlags to not duplicate the magic number (reference: function mongoFlagsCommandFlags, flag name "documentdb-sampling-fanout", NewConn and setDefault, and mongo.defaultDocumentDBSamplingFanoutLimit).connectors/mongo/conn.go (1)
249-249: Note the multiplicative fanout for DocumentDB.
eg.SetLimit(c.planningFanoutLimit())bounds concurrent namespace planning, but each planning goroutine callsc.sampleIDs, which (for the DocumentDB flavor) opens its own errgroup bounded bydocumentDBSamplingFanout(). With both defaulting to 100, peak concurrency reaches ~100 × 100 = 10000simultaneous$sampleaggregations against the same cluster, which can exhaust the driver connection pool and pressure the server. This is still a large improvement over the previously unbounded fanout, so no change is required, but consider whether the effective product should be capped or documented.Also applies to: 300-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@connectors/mongo/conn.go` at line 249, The current concurrency bounding uses eg.SetLimit(c.planningFanoutLimit()) but each planner goroutine calls c.sampleIDs which itself creates an inner errgroup bounded by documentDBSamplingFanout(), producing a multiplicative fanout (eg.SetLimit × documentDBSamplingFanout) that can overwhelm the driver; update the logic so the effective parallelism is capped or controlled—either reduce one of the limits, enforce a global cap (e.g., compute min(c.planningFanoutLimit(), maxGlobalSampling/concurrentSampleLimit) before calling eg.SetLimit), or add coordination so c.sampleIDs uses a shared semaphore instead of its own uncoordinated errgroup; locate and modify eg.SetLimit(...) and c.sampleIDs (and documentDBSamplingFanout()) to implement the chosen cap or add documentation explaining the multiplicative effect and recommended defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@connectors/mongo/conn.go`:
- Line 249: The current concurrency bounding uses
eg.SetLimit(c.planningFanoutLimit()) but each planner goroutine calls
c.sampleIDs which itself creates an inner errgroup bounded by
documentDBSamplingFanout(), producing a multiplicative fanout (eg.SetLimit ×
documentDBSamplingFanout) that can overwhelm the driver; update the logic so the
effective parallelism is capped or controlled—either reduce one of the limits,
enforce a global cap (e.g., compute min(c.planningFanoutLimit(),
maxGlobalSampling/concurrentSampleLimit) before calling eg.SetLimit), or add
coordination so c.sampleIDs uses a shared semaphore instead of its own
uncoordinated errgroup; locate and modify eg.SetLimit(...) and c.sampleIDs (and
documentDBSamplingFanout()) to implement the chosen cap or add documentation
explaining the multiplicative effect and recommended defaults.
In `@internal/app/options/connectorflags.go`:
- Around line 734-741: The hidden flag in mongoFlagsCommandFlags currently
hardcodes Value: 100 for "documentdb-sampling-fanout"; instead either remove the
Value so the connector's NewConn/setDefault logic
(mongo.defaultDocumentDBSamplingFanoutLimit) remains the single source of truth,
or reuse/export that constant for the flag's default; update
mongoFlagsCommandFlags to not duplicate the magic number (reference: function
mongoFlagsCommandFlags, flag name "documentdb-sampling-fanout", NewConn and
setDefault, and mongo.defaultDocumentDBSamplingFanoutLimit).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d46922df-f4ee-4c39-bc23-9bd0705aecb0
⛔ Files ignored due to path filters (3)
gen/adiom/commands/connectors/v1/cosmos_commandargs.gois excluded by!**/gen/**gen/adiom/commands/connectors/v1/mongo.pb.gois excluded by!**/*.pb.go,!**/gen/**gen/adiom/commands/connectors/v1/mongo_commandargs.gois excluded by!**/gen/**
📒 Files selected for processing (5)
connectors/mongo/conn.goconnectors/mongo/conn_unit_test.goconnectors/mongo/docdb.gointernal/app/options/connectorflags.goproto/connectorcommands/adiom/commands/connectors/v1/mongo.proto
Summary by CodeRabbit
New Features
Tests